Skip to content

ZEPPELIN-511 REST API: Insert / Retrieve / Move / Delete operation for paragraph #550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
HeartSaVioR wants to merge 4 commits into from
Closed

ZEPPELIN-511 REST API: Insert / Retrieve / Move / Delete operation for paragraph #550

HeartSaVioR wants to merge 4 commits into from

Conversation

@HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Dec 18, 2015

What is this PR for?

This issue is intended to fill gap between REST API and WebSocket operations.
For now we can only access notebook and paragraph to READONLY (not writing) via REST API but after this PR, we can insert / retrieve / move / delete paragraph via REST API.

What type of PR is it?

Feature

Todos

Is there a relevant Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-511

How should this be tested?

Please follow the explanation of added REST APIs from rest-notebook.md.

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? (No)
  • Is there breaking changes for older versions? (No)
  • Does this needs documentation? (Yes, I've addressed it.)

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Dec 21, 2015

Thanks for adding really useful REST APIs!
Looks good to me!

prabhjyotsingh
prabhjyotsingh reviewed Dec 21, 2015
View reviewed changes
zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java Outdated
Copy link
Contributor

@prabhjyotsingh prabhjyotsingh Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable should be p2

Copy link
Contributor Author

@HeartSaVioR HeartSaVioR Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prabhjyotsingh Good finding! Will fix.

@prabhjyotsingh
Copy link
Contributor

prabhjyotsingh commented Dec 21, 2015

@HeartSaVioR few more things

I think you should do a clean up as well after every test case execution

ZeppelinServer.notebook.removeNote(note.getId());

Also when I run this, line 538;

PostMethod post = httpPost("/notebook/" + note.getId() + "/paragraph/" + p.getId() + "/move/" + 10, "");

it returns

{"status":"OK","message":""}

Which i think is also not appropriate. Since, this was an invalid operation (total nos of paragraph = 2). This should have either returned some error, or should have moved 1st paragraph to the last location.

prabhjyotsingh
prabhjyotsingh reviewed Dec 21, 2015
View reviewed changes
zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java Outdated
Copy link
Contributor

@prabhjyotsingh prabhjyotsingh Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be also post2.

Copy link
Contributor Author

@HeartSaVioR HeartSaVioR Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding, will fix.

@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Dec 21, 2015

@prabhjyotsingh
Thanks for the review. Addressed your comments.
Regarding moving paragraph to invalid index, I think it could be treated as bad request, so I applied it. Would it be fine for you?

@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Dec 22, 2015

Upmerged. Please take a look again, thanks!

@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Dec 24, 2015

Done with another upmerge.

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Dec 26, 2015

Looks good to me.
Merge if there're no more discussions.

@minahlee
Copy link
Member

minahlee commented Dec 26, 2015

Would you mind to do rebase than merge? It will be easier to keep track who has credits on merged code.

@HeartSaVioR
ZEPPELIN-511 REST API: Insert / Retrieve / Move / Delete operation fo…
3658b00 
…r paragraph

* implements 4 REST APIs about the paragraph
* Added unit test to test this features

Conflicts:
	zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java
@HeartSaVioR
ZEPPELIN-511 "insert paragraph" api: return created paragraph id
cc71e06 
* also change http status code to 201 CREATED
@HeartSaVioR
ZEPPELIN-511 explain new APIs to read-notebook.md
01feed4
@HeartSaVioR
ZEPPELIN-511 Address @prabhjyotsingh reviews
59f5f61 
Conflicts:
	zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Dec 26, 2015

@minahlee No problem. Rebased via creating new branch, cherry-pick, and reset branch's head.

@minahlee
Copy link
Member

minahlee commented Dec 27, 2015

@HeartSaVioR Thanks for quick response. LGTM

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Dec 27, 2015

LGTM. Merging it into master

@asfgit asfgit closed this in 3bfd97e Dec 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@HeartSaVioR @Leemoonsoo @prabhjyotsingh @minahlee